Conversation
… and Kora's price model. Also removed check for non transferable and cpi guard, as they will fail at the transaction level instead. - Introduced `calculate_transfer_fees` method to compute transfer fees for token transactions. - Updated `estimate_transaction_fee` to return a `TotalFeeCalculation` struct, encapsulating all fee components. - Removed forced non transferable and cpi guard check, will be validated by the chain when the transaction is submitted.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 129907c in 2 minutes and 22 seconds. Click for details.
- Reviewed
974lines of code in9files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/validator/transaction_validator.rs:40
- Draft comment:
Configuration in new() is built by converting allowed/disallowed program/token strings. Consider adding inline comments documenting the rationale for using Pubkey::from_str so future maintainers know these are from trusted config sources. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
2. crates/lib/src/validator/transaction_validator.rs:172
- Draft comment:
In the closure 'check_if_allowed', the error message 'Fee payer cannot be used as source account' might be too generic. Consider including the offending account or more context for easier debugging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. crates/lib/src/validator/transaction_validator.rs:160
- Draft comment:
Downcasting of token mint state and token account state (in functions called in validate_token_payment and others) use unwrap(). If a downcast fails unexpectedly, this may cause a panic. Consider using proper error handling instead of unwrap. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. crates/lib/src/validator/transaction_validator.rs:262
- Draft comment:
The validate_disallowed_accounts function iterates over each instruction and its accounts. Consider refactoring or documenting the logic for clarity, as nested loops can be hard to follow. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
5. crates/lib/src/validator/transaction_validator.rs:294
- Draft comment:
The function calculate_total_outflow simply delegates to FeeConfigUtil::calculate_fee_payer_outflow. Ensure that the outflow calculation properly handles edge cases (e.g. when fee payer is not involved, inflows are subtracted using saturating_sub, etc.). - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. crates/lib/src/validator/transaction_validator.rs:298
- Draft comment:
In validate_token_payment, the error message simply states 'Insufficient token payment. Required {required_lamports} lamports'. Consider including actual computed payment versus requirement for improved diagnostics. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
7. crates/lib/src/validator/transaction_validator.rs:400
- Draft comment:
Test coverage in this module is extensive. It might be useful to add comments describing the intention behind each fee payer policy test case to aid future maintainers. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
8. crates/lib/src/fee/fee.rs:1324
- Draft comment:
Typo: The format string in the error message is inconsistent. It uses '{}' for the first parameter but '{expected_min_fee}' for the second. Consider using '{}' for both placeholders (e.g., "Got: {}, Expected at least: {}") and passing both fee.total_fee_lamports and expected_min_fee accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor formatting issue in a test file. The code works correctly either way - both formatting styles are valid Rust. The comment is purely about code style consistency. According to the rules, we should not make comments that are obvious or unimportant. This seems to fall into that category. Could this inconsistency cause confusion for future developers? Could it propagate inconsistent style throughout the codebase if left unchecked? While consistency is good, this is a very minor style issue in a test file. The code functions correctly either way. The benefit of fixing this is minimal compared to the overhead of reviewing and addressing the comment. Delete this comment. It points out a very minor style inconsistency that doesn't affect functionality, in a test file. This falls under the rule about not making obvious or unimportant comments.
Workflow ID: wflow_8FafSG3kAu1LssXq
You can customize 
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0529c96 in 52 seconds. Click for details.
- Reviewed
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/fee/fee.rs:165
- Draft comment:
Consider adding a doc comment for the newfee_payerparameter incalculate_transfer_feesto explain its role and relation to the transaction's fee payer. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. crates/lib/src/fee/fee.rs:165
- Draft comment:
Ensure that the providedfee_payermatches the fee payer in the transaction (previously taken as the first key) to avoid inconsistencies. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that thefee_payermatches the fee payer in the transaction. This is a request for confirmation of behavior, which violates the rule against asking the author to ensure behavior is intended.
3. crates/lib/src/fee/fee.rs:275
- Draft comment:
The call tocalculate_transfer_feesis correctly updated to includefee_payer. Verify that all call sites (including tests) provide the intended fee payer. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_3tof9Cw9SxUmrh94
You can customize
Contributor
|
@dev-jodee if you're good with it, can you merge #214 into this PR before merging? |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 25f9d0a in 1 minute and 56 seconds. Click for details.
- Reviewed
259lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/fee/fee.rs:247
- Draft comment:
calculate_transfer_fees returns immediately on the first eligible Token2022 transfer fee. If multiple transfers can incur fees, consider summing them rather than returning early. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. crates/lib/src/fee/fee.rs:147
- Draft comment:
In get_payment_instruction_info, when an account is missing and skip_missing_accounts is true, consider logging this case to aid debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a suggestion about adding logging, which is generally a code quality improvement. However, the current error handling is intentional - it's not an error case that needs debugging, but rather an expected flow when skip_missing_accounts is true. The function is working as designed by returning None when accounts don't exist and skipping is enabled. Adding logging here would add noise to the logs without providing meaningful debugging value. The comment could be right that having more visibility into skipped accounts could help with debugging in some scenarios. Maybe there are edge cases where knowing which accounts were skipped would be valuable. While logging can be helpful, in this case the skipping behavior is an intentional part of the function's design, not an error condition that needs investigation. The function name and documentation clearly indicate this is expected behavior. The comment should be deleted. The suggestion to add logging is not addressing a real issue, as the current behavior is intentional and well-documented. Adding logging here would be unnecessary noise.
3. crates/lib/src/rpc_server/method/estimate_transaction_fee.rs:70
- Draft comment:
The integration of calculate_fee_in_token is clean; ensure that any rounding or conversion details between lamports and token values are documented if needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_0zXJjWdUbdCk9jKT
You can customize
dev-jodee
deleted the
bugfix/pro-268-estimate-txn-fee-needs-to-include-transfer-fees
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…s and Kora's price model. Also removed check for non transferable and cpi guard, as they will fail at the transaction level instead.
calculate_transfer_feesmethod to compute transfer fees for token transactions.estimate_transaction_feeto return aTotalFeeCalculationstruct, encapsulating all fee components.Important
Enhances fee estimation by adding transfer fee calculation and updating the fee structure, while removing certain checks now handled at the transaction level.
calculate_transfer_feesinfee.rsto compute transfer fees for token transactions.estimate_transaction_feeto returnTotalFeeCalculationstruct with detailed fee components.TotalFeeCalculationstruct infee.rsto encapsulate fee components.estimate_kora_feeto apply Kora's price model.calculate_fee_in_tokento convert fees to specific tokens.fee.rsandestimate_transaction_fee.rsto reflect new fee calculation logic.FEES.mdto document fee estimation components and calculation methods.README.mdto include fee estimation guide reference.This description was created by
for 25f9d0a. You can customize this summary. It will automatically update as commits are pushed.
📊 Test Coverage
Coverage: 86.3%
View Detailed Coverage Report